❔ what's this for

editor automatically fixing superfluous eslint rules

❔ Should check https://devdocs.io/dom/abortsignal/aborted before .abort?

The signal only seems to raise an abort event once regardless of how many times you call abort, so it's probably fine.

var ac = new AbortController();
var signal = ac.signal;
signal.addEventListener('abort', ()=>{ console.log('aborted') });
ac.abort();
ac.abort();//only logs `abort` once

❔ This runs when a signal aborts right? Seems like we should have a distinct catch for AbortError and an action like appReceivedAbortSignalDuringResolverDataFetch

seems like a good idea. i'm going to try that.

Something like:

      } catch (error) {
        // https://developer.mozilla.org/en-US/docs/Web/API/DOMException#exception-AbortError
        if (error instanceof DOMException && error.name === 'AbortError') {
          api.dispatch({
            type: 'appAbortedResolverDataRequest',
            payload: databaseDocumentIDToFetch,
          });
        } else {
          api.dispatch({
            type: 'serverFailedToReturnResolverData',
            payload: databaseDocumentIDToFetch,
          });
        }
      }

?

@bkimmel i moved your tests here

this was returning an object with the store in it.

If we use 'visibility' here, it still occupies space. The parent is 'flex' and Resolver is a sibling of this component. We need Resolver to grow. Seems like display: none did the trick. Anyone know of any reason why this would be bad?

@XavierM seems to know many things

@jonathan-buttner if ya get a chance, could ya give it a quick look?

Do we also need to mock the new style endpoint events in ecs?

I don't think it should matter here. The code operates on the ResolverEvent interface which supports either old or new events. As long as the methods works correctly on old and new style events, this code should work. We could also test those methods in isolation.

At some point, old events and new ECS events will probably stop being equivalent to each other. At that point we'd probably want a version of mockProcessEvent that returns the new style as well. At that point, if we wanted to be exhaustive, we could make a version of the mockProcessEvent that returned new ECS events and then run these tests w/ those objects as well.

Does that all add up?

Since most of these tests are for code paths that occur after we normalize unique_pid/entity_id I don't think it's super important now

Sounds good, just wanted to double check 👍

@bkimmel don't know if it matters but the generated tree will have parents with up to 2 children (randomly generated). If you always want the nodes to have exactly 2 children you can pass in alwaysGenMaxChildrenPerNode: true

I think we should do that

Do we need to handle this scenario in this PR?

abort is handled. TODO comment should be removed. ty

Hopefully we can bump these number up after I do some performance testing.

I think we can switch this to an IDs search: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-ids-query.html

@andrew-goldstein the _id that we're passing should be the unique ES document ID (i.e. _id field right) right?

I'll give this a shot

worked just fine.

Should we try/catch around this? If ES fails for some reason maybe the server should log and return a 500?

Let me look into this.

As the code is written now:

Logs in Kibana:

And the response was: {"statusCode":500,"error":"Internal Server Error","message":"An internal server error occurred."}

This seems fine to me. The internal details are logged, while the client gets a generic 500.

👍 Sounds good, thanks for looking.

🤷 Resolver spelled wrong in the comment

Intsead and predicatable too. (i turned on spellcheck lol)

👌